[AMDGPU] Error out in clang if wavefront64 is used on gfx1250 #153693
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-amdgpu Author: Stanislav Mekhanoshin (rampitec) ChangesFull diff: https://github.com/llvm/llvm-project/pull/153693.diff 2 Files Affected:
diff --git a/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl b/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
index 4e2f7f86e8402..04de5dca3f6c0 100644
--- a/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
+++ b/clang/test/CodeGenOpenCL/amdgpu-features-illegal.cl
@@ -1,8 +1,10 @@
// RUN: not %clang_cc1 -triple amdgcn -target-feature +wavefrontsize32 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s
// RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx1103 -target-feature +wavefrontsize32 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s
// RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx900 -target-feature +wavefrontsize32 -o /dev/null %s 2>&1 | FileCheck %s --check-prefix=GFX9
+// RUN: not %clang_cc1 -triple amdgcn -target-cpu gfx1250 -target-feature +wavefrontsize64 -o /dev/null %s 2>&1 | FileCheck %s --check-prefix=GFX1250
// CHECK: error: invalid feature combination: 'wavefrontsize32' and 'wavefrontsize64' are mutually exclusive
// GFX9: error: option 'wavefrontsize32' cannot be specified on this target
+// GFX1250: error: option 'wavefrontsize64' cannot be specified on this target
kernel void test() {}
diff --git a/llvm/lib/TargetParser/TargetParser.cpp b/llvm/lib/TargetParser/TargetParser.cpp
index 50b97d3257540..31558126c66b3 100644
--- a/llvm/lib/TargetParser/TargetParser.cpp
+++ b/llvm/lib/TargetParser/TargetParser.cpp
@@ -774,6 +774,18 @@ static bool isWave32Capable(StringRef GPU, const Triple &T) {
return IsWave32Capable;
}
+static bool isWave64Capable(StringRef GPU, const Triple &T) {
+ if (T.isAMDGCN()) {
+ switch (parseArchAMDGCN(GPU)) {
+ case GK_GFX1250:
+ return false;
+ default:
+ break;
+ }
+ }
+ return true;
+}
+
std::pair<FeatureError, StringRef>
AMDGPU::insertWaveSizeFeature(StringRef GPU, const Triple &T,
StringMap<bool> &Features) {
@@ -788,6 +800,9 @@ AMDGPU::insertWaveSizeFeature(StringRef GPU, const Triple &T,
if (HaveWave32 && !IsNullGPU && !IsWave32Capable) {
return {AMDGPU::UNSUPPORTED_TARGET_FEATURE, "wavefrontsize32"};
}
+ if (HaveWave64 && !IsNullGPU && !isWave64Capable(GPU, T)) {
+ return {AMDGPU::UNSUPPORTED_TARGET_FEATURE, "wavefrontsize64"};
+ }
// Don't assume any wavesize with an unknown subtarget.
if (!IsNullGPU) {
// Default to wave32 if available, or wave64 if not
|
| } | ||
|
|
||
| static bool isWave64Capable(StringRef GPU, const Triple &T) { | ||
| if (T.isAMDGCN()) { |
There was a problem hiding this comment.
This should be added to the feature flags in the big table above, and not require multiple parse + switch functions for different cases
There was a problem hiding this comment.
Do you want to add a new feature to every target out there?
There was a problem hiding this comment.
I agree with Matt. We already have fillAMDGPUFeatureMap() that is supposed to fill the default features. On line 434/435, it already fills wave32/64 for spirv. It is kind of redundant and may cause inconsistency to have isWave32Capable() and isWave64Capable(). If we need a way to get wave64 capability, we could cache the default target features returned by fillAMDGPUFeatureMap() in TargetInfo and look it up for wave64 capability.
There was a problem hiding this comment.
Everything should be feature driven, not random GPU check driven. It's not sustainable especially when features get removed
There was a problem hiding this comment.
So to clarify, do you suggest adding a wavefrontsize32 or wavefrontsize64 feature in the fillAMDGPUFeatureMap to every target which can only support one wavefront size? And then if it is in the Features map assume the target cannot support another?
There was a problem hiding this comment.
Ok, but as usual adding features to older targets will be invasive and disruptive.
There was a problem hiding this comment.
Turns out it is not so bad as wavefront size was inserted anyway, just at a different point.
|
ping |
rampitec
deleted the
users/rampitec/08-14-_amdgpu_error_out_in_clang_if_wavefront64_is_used_on_gfx1250
branch
No description provided.